Skip to content

Conversation

@SherryShijiarui
Copy link

No description provided.

Copy link
Contributor

@A-lexisL A-lexisL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. tests should be comprehensive, so more tests are needed. Please arrange them into classes based on API and Authenticated status.
    e.g.
class TestCourseListAuthenticated
class TestCourseListNotAuthenticated
class TestCourseDetailAuthenticated
...

response = base_client.get(url)
assert response.status_code == 200
# Should be at least 1 due to the 'review' fixture
assert response.data["review_count"] >= 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test should be precise, so use ==1 instead of >=1

response = base_client.get(url)

assert response.status_code == 200
assert response.data["count"] >= 3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

# Check that filtering worked (only 1 course returned)
assert response.data["count"] == 1

# FIX: Check course_code instead of department key
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that you are making adjustments during developments. But comments are for reviewers and future maintainers, so the Fix: xxx here becomes confusing

assert "results" in response.data

def test_filter_courses_by_department(self, base_client, course_factory):
"""Verify filtering courses by department code."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, only filter courses by department code for nonauthenticated user exists. But tests for all the following params are needed for authenticated/nonauthenticated users.

            - department (string): Filter by department code (case-insensitive)
            - code (string): Filter by course code (partial match)
            - min_quality (integer): Filter by minimum quality score (authenticated only)
            - min_difficulty (integer): Filter by minimum difficulty score (authenticated only)
            - sort_by (string): Sort field ("course_code", "review_count"),("quality_score", "difficulty_score")(authenticated only)
            - sort_order (string): "asc" or "desc" (default: "asc")
            - page (integer): Page number for pagination

(docstring of CoursesListAPI in apps/web/views.py)

Same for other apis

@factory.lazy_attribute
def course_code(self):
"""Generates unique MATH100, PHYS101, etc."""
return f"{self.department}{self.number}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

course_code is department+4-digit number+J, e.g. CHEM2110J

@pytest.fixture
def course_factory(db):
"""Fixture to access the factory class directly for batch creation"""
return factories.CourseFactory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since more tests are needed, if the same group of db instances are used repeatedly, could consider wrap them up in fixture. e.g.

@pytest.fixture
def course(db):
    return factories.CourseFactory()

@pytest.fixture
def course_batch(db):
    return factories.CourseFactory.create_batch(3)
@pytest.fixture
def department_mixed_courses(db):
    courses = [
        CourseFactory(department="MATH", course_title="Honors Calculus II",course_code="MATH1560J"),
        CourseFactory(department="MATH", course_title="Calculus II",course_code="MATH1160J"),
        CourseFactory(department="CHEM", course_title="Chemistry",course_code="CHEM2100J"),
        # and more 
    ]
    return courses

class TestReviewManagement:
"""
Tests for Review management:
- Creation with validation (30+ chars, valid term)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

30 is not a fixed number but varies according to configs.
Its best to generate comments based on settings.WEB["REVIEW"]["COMMENT_MIN_LENGTH"] (since the content of comments is not important, could use python internal string manipulation methods or faker to generate comments of desired length instead of writing them manually)

url = reverse("user_review_api", kwargs={"review_id": review.id})
response = auth_client.delete(url)
assert response.status_code == 204
assert Review.objects.filter(id=review.id).count() == 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing tests for base_client


# Should be 400 according to standard API validation rules
assert response.status_code == 400

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing canceling vote/changing vote/vote for difficulty.. tests


assert response.status_code == 200
# Verify the title matches the fixture-created course
assert response.data["course_title"] == course.course_title
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

base_client and auth_client will receive different response data:

  • auth_client: full details
  • base_client: "quality_score" ,"difficulty_score","difficulty_vote","quality_vote","quality_vote_count","difficulty_vote_count" fields are removed(the docstrings for CoursesDetailAPI is not complete)

@A-lexisL A-lexisL moved this from Todo to In Progress in Course Review Jan 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

5 participants